fix: resolve multiple bugs and code quality issues#57
Conversation
- Fix critical `in` operator bug in refreshProxy() — `proxyType in ["system", "direct"]` always returned false; replaced with `["system", "direct"].includes(proxyType)` - Fix service worker crash: wrap `new URL(details.url)` in try/catch in background.ts - Fix document.body null access in preference.ts using optional chaining - Replace deepClone() JSON.parse/JSON.stringify with structuredClone() for correctness - Fix window.open() called with import.meta.url as target name; use '_blank' instead - Replace .map() with .forEach() for side-effect-only iterations (profile.ts, auth.ts) - Remove leftover debug console.log calls from ThemeSwitcher, AutoSwitchInput, background, PopupPage - Fix i18n typo: "Advance Config" → "Advanced Config" Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a comprehensive set of bug fixes and code quality improvements across various parts of the application. It addresses critical logical errors, enhances the stability of the service worker, prevents potential runtime crashes, and refines core utility functions. The changes also include correcting user interface behaviors and cleaning up development artifacts, leading to a more robust, reliable, and maintainable codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a good number of bugs and code quality issues, from critical logic errors like the misuse of the in operator to robustness improvements and removal of debug code. The changes are well-described and the fixes are generally excellent.
I've found one critical issue with the switch to structuredClone for deepClone. While structuredClone is generally superior, it cannot handle Vue's reactive Proxy objects, which contradicts the documented purpose of the deepClone function in this codebase. This change could lead to runtime errors when saving profiles. I've left a specific comment with a suggestion to revert this particular change.
| */ | ||
| export function deepClone<T>(obj: T): T { | ||
| return JSON.parse(JSON.stringify(obj)); | ||
| return structuredClone(obj); |
There was a problem hiding this comment.
The switch to structuredClone() introduces a critical issue. The documentation for deepClone states its purpose is to 'remove all Proxy objects (e.g., from Vue reactivity)', which is necessary before saving to chrome.storage.
However, structuredClone() cannot clone Proxy objects and will throw a DataCloneError. The previous JSON.parse(JSON.stringify()) implementation correctly handled this by serializing the underlying raw object from the proxy.
While some call sites might use toRaw() before cloning, others (like in profile.ts) may not, leading to runtime errors when saving profiles. Given this, I recommend reverting to the previous implementation to ensure reactive objects are handled correctly.
| return structuredClone(obj); | |
| return JSON.parse(JSON.stringify(obj)); |
structuredClone() throws a DataCloneError on JavaScript Proxy objects, including Vue reactive() and ref() wrappers. The JSON.parse/JSON.stringify approach correctly serializes through the Proxy traps, producing a plain object safe for chrome.storage. Extend the comment to document why structuredClone() is intentionally avoided here, to prevent this mistake in future. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Provides build/test commands, architecture overview (adapter layer, PAC-via-AST proxy engine, profile system), and critical gotchas like the deepClone JSON round-trip requirement. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 participant
Summary
refreshProxy()used theinoperator on an array (proxyType in ["system", "direct"]), which checks for index keys (0, 1), not values — alwaysfalse. Replaced with.includes().new URL(details.url)inbackground.tsthrew on malformed URLs, silently killing the service worker. Wrapped in try/catch.document && document.body.setAttribute(...)inpreference.tsguardeddocumentbut notdocument.body. Replaced with optional chaining.JSON.parse(JSON.stringify())—structuredClone()throwsDataCloneErroron Vue Proxy objects.PopupPage.vuepassedimport.meta.urlas the window target name instead of'_blank'..map()used for side effects inprofile.tsandauth.ts→ replaced with.forEach().newProxyString()inscriptHelper.tscompared the unnormalizedcfg.schemeinstead of the defaultedschemevariable, producing wrong proxy strings when scheme was undefined.console.log/console.debugcalls removed fromThemeSwitcher.vue,AutoSwitchInput.vue,background.ts,PopupPage.vue, andstats.ts."Advance Config"→"Advanced Config".createRequireformanifest.json.@sentry/vueand@sentry/vite-plugin, reducing main.js bundle by ~32%.failedRequestsCountgetter, stale TODO comments, and obsoletebuild:testscript.var→constinpacSimulator.ts.Test plan
npx vitest run) — up from 44deepClone,LRUCache,shExpMatch,isPlainHostName,newProxyStringfindProfiletests with precise profileID assertionsnpm run buildandnpm run build:firefoxboth succeedvue-tsc --noEmitpasses with zero errorsin→includesfix🤖 Generated with Claude Code